Skip to content

Conversation

isc-cge
Copy link
Collaborator

@isc-cge isc-cge commented Jul 5, 2024

Coverage tracking results now include embedded python methods in .CLS files

Implements #29

isc-cge added 11 commits June 24, 2024 15:31
…ts in a python list, started and stopped along with the LineByLine Monitor
…tores the MethodMap (line numbers for each method), and executable lines got a bug fix. UpdateSourceMap now saves mapped line numbers from .py to .cls
…PCJG (as well as code that unsuccessfully tries to save it into SQL)
…then wrote StorePyCoverage to store that data in a Coverage
Added ..CoveragePythons

Changed AddCoverageClass to add to ..CoveragePythons

Added ..PyCoverageTargets

Changed SetCoverageTargets (which is called when the user actually passes in coverage classes and routines, instead of it being in coverage.list) to fill in PyCoverageTargets with just the non .py part of the class name

Got UpdateCoverageTargetsForTestDirectory to update ..PyCoverageTargets in the case where there’s a coverage.list

Handle StartCoverageTracking

seems redundant to include the python in the new / relevant stuff: it’s new or relevant if its class is new / relevant

Added the python components of the old relevant classes into tPyRelevantTargets

With that being said, I think we need Snapshot to return the python targets for the relevant classes

In Snapshot: checked if the class has a python part (if it does, it’s relevant), and added those to the list of python new relevant targets. Took a snapshot of each of those

Combined tPyNewRelevantTargets into tPyRelevantTargets
…erageTracking to StoreIntRoutine to store the coverage results from the .py CodeUnits, resulting in python lines being marked as covered in .cls files
Copy link
Collaborator

@isc-tleavitt isc-tleavitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in weekly touchpoint today:

  1. Parallelize taking the snapshot of the Python routines and fetching the executable lines back to the .cls files -> no need for this
  2. Do we want to store the Python trace data differently (right now it’s just in the global ^IRIS.TEMPCJG) -- if so, need to fix the crashing error that I never resolved - this is a fine approach, just use a better global name
  3. What is cyclomatic complexity, and what are we using it for? Will need to implement for python CodeUnits probably - using radon with requirements.txt and IPM's support for python dependency installation should work
  4. Right now, CodeUnit’s LineToMethodMap property (already existed) and the CoveragePythons / PyCoverageTarget properties in Manager (I added) aren’t doing anything. Keep or get rid of them? - LineToMethodMap is needed for .INT/.CLS, let's keep all for consistency
  5. Manually change some of the lines to not executable: for instance, it seems weird that the class method definition gets marked as executed even if the class method is never called (but sys.settrace does show it as executed) - only really matters for class method definition, solution is to not mark as executable
  6. Write unit tests for the new python code - great idea!
  7. Use of $Namespace vs. not in different contexts - good question, see #13 (which is pretty useless)
  8. requirements.txt example - see Anya's work

@isc-cge isc-cge changed the title Draft Pull Request EP Embedded Python Coverage Support Jul 8, 2024
isc-cge added 2 commits July 8, 2024 13:50
Turns out there's no .INT routine generated for .CLS files with only embedded python,
which means that we can no longer piggyback finding out which python files to track
by using the .CLS files. I now keep track of relevant and new python coverage targets separately
Copy link
Collaborator

@isc-tleavitt isc-tleavitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have relatively few comments and most of them are more stylistic than a matter of correctness - very well done!!

CHANGELOG.md Outdated
@@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [3.2.0] - 2024-07-08
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should say Unreleased until released

isc-shuliu
isc-shuliu previously approved these changes Jul 17, 2024
Copy link
Contributor

@isc-shuliu isc-shuliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed today, we can look into aligning the start of python methods in subsequent PRs.

@isc-shuliu
Copy link
Contributor

As discussed today, we can look into aligning the start of python methods in subsequent PRs.

Turns out to be a non-issue :)

isc-tleavitt
isc-tleavitt previously approved these changes Jul 31, 2024
Copy link
Collaborator

@isc-tleavitt isc-tleavitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go - just need to clean up CHANGELOG.md to reflect all under 3.2.0 (including listeners which got merged here, fine if that goes in with this PR too to save some time)

@isc-tleavitt isc-tleavitt merged commit f7ccc52 into master Aug 1, 2024
3 checks passed
@isc-cge isc-cge deleted the EmbeddedPython branch August 9, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants